cucumber-cpp support for cgreen test framework. #129

shnewto · 2016-11-16T00:53:11Z

As a user I want to use cgreen as an alternative test framework to gtest
and boost in order to test native C complete with mocking capability.

cucumber-cpp now recognizes and harnesses cgreen as a testing framework.
The cgreen reporting is intercepted and presented on the failure of a
cucumber
test.

An example (FizzBuzz) is written as an example of a testable C progam
and
is included in the examples directory.

Unit tests for the cgreen driver are modeled after the boost and gtest
drivers and are passing.

shnewto · 2016-11-18T21:04:38Z

Cgreen repo lives here.

paoloambrosio · 2016-11-22T07:02:51Z

Thanks for submitting this PR! :-) I've been quite busy to do an in-depth review but I'll do it ASAP.

In the meantime some observations that will keep you busy:

The CI build doesn't seem to include it
The code style is sometimes inconsistent with the rest of the code base (e.g. mixed camelCase and underscore_case, "char*" VS "char *", brackets before or after a new line, ...)
No copyright information please
No blank lines at the beginning of files

shnewto · 2016-11-23T18:06:02Z

Thanks for the update!

I've addressed the following in this PR:

CgreenDriver and FizzBuzz example integrations into the CI builds.
Consistency in code style.

Maintaining the copyright information is a hard requirement for me though.

Although the project reports "Copyright (c) 2010 Paolo Ambrosio"
my understanding is that copyright of contributors is still implicit
and recorded by Github's record of project participants.

Unfortunately the role of PolySync Technologies, Inc. is not
reflected in that report and it needs to be somehow. I'd be
willing to put some time into a project contributor's page if
that's a more palatable option.

paoloambrosio · 2016-11-25T08:58:46Z

No other source file in the code base is polluted with copyright information, and we are not going to start now. I'm more than happy to accept PRs from "PolySync Technologies, Inc.", but we'll have to reach an agreement before this PR is considered for merging.

The license chosen is among the most permissive in the OSS world. I'm surprised to receive this request from a company that has open-sourced software. Does it mean that the company would accept in its project a contribution assigning copyright to me?

Contributors are recorded in the Changelog file when merging each PR, as well as being in the Git logs. The role of "PolySync Technologies, Inc." is contributing with that PR, and I have no problems attributing the change to the company instead of you. If this is agreeable, we don't need to discuss it anymore :-)

If that is not enough, I think we should discuss this in the https://gitter.im/cucumber/contributors channel and hear the opinion of the others core contributors.

aslakhellesoy · 2016-11-25T09:54:03Z

Each repository under the cucumber GitHub organisation should have one and only one legal entity as copyright holder.

Most of the repositories under the "Cucumber" GitHub organisation have an MIT license where the copyright holder is Cucumber Ltd. For historical reasons the copyright holder of this repo is Paolo Ambrosio.

Cucumber Ltd (who owns the Cucumber GitHub organisation) will not allow more than one copyright holder for Cucumber Ltd repos, and I recommend the same policy for repositories like this one.

If a project wants to stray from this recommendation they would have to fork and move outside the Cucumber organisation, something I'm sure @paoloambrosio isn't planning on ;-)

So yes, these Copyright PolySync Technologies, Inc. lines must be removed before this can be merged.

shnewto · 2016-11-25T19:57:06Z

Ah that changelog file works perfectly! I'd missed that. My concerns will be wholly addressed by attributing changes to Shea Newton @ PolySync Technologies, Inc. there. Thanks for your time and patience, I really appreciate this project, am happy to to use it and am excited for the opportunity to extend it.

shnewto · 2016-11-25T19:58:27Z

Copyright info removed from PR.

aslakhellesoy · 2016-11-25T22:56:24Z

Thanks for your understanding @Snewt - and glad you're happy with an honorable mention in the changelog!

And thanks for your contribution.

Cheers,
Aslak

paoloambrosio · 2016-12-08T19:01:49Z

I'm slowly going through this PR (apologies for that).

It took me some time to realise that it doesn't work with the released version of Cgreen, that doesn't even come with CMake config files. I know, I missed that your contribution requires "Cgreen 1.1.0 or later"... but in my defence that version does not exist!

Also Cgreen's CMake config in the master branch doesn't export the library's full path, making my local build fail since I keep Cucumber-CPP's dependencies in a separate directory (solved exporting the LIBRARY_PATH variable). I will consider contributing back to cgreen the more standard CMake config file that I just wrote.

I also probably found the reason why you didn't add cgreen to the AppVeyor build for Windows: cgreen does not compile with Visual Studio, and indeed they don't build on Windows. Again, spent time solving a few issues but I gave up after a while (I don't even like Windows!).

I like that you added the FizzBuzz example to test a pure C application. I would also create a CgreenCalculatorSteps.cpp file in examples/Calc/features/step_definitions to show how to use it with C++. That directory in my view should contain one file per test framework supported.

More to follow after I take a look at the actual code ;-)

paoloambrosio

Still need to give it a better read to understand how it works.

I've left a few comments in this review.

On top of those, code conventions are inconsistent:

underscores => camel case
if(...)\n{ => if (...) { (except in CMake files)
same for for statements

paoloambrosio · 2016-12-18T22:09:22Z

CMakeLists.txt

+# cgreen
+#
+if(NOT CUKE_DISABLE_CGREEN)
+ find_package(cgreen QUIET)


QUIET is not supported by cgreen-config.cmake, and even if it was all other find_package in the project do not use it

QUIET is a cmake directive, it allows the cmake step to succeed if the find_package function fails to find cgreen. Without the QUIET directive cmake can fail there. I'm happy to remove it if you want cmake to fail if the package is not found.

QUIET suppresses both success and failure messages. For consistency with the other find_package calls, until , I suggest adding a more standard Findcgreen.cmake.

Feel free to pull this commit on my personal repository that adds it on top of your PR.

paoloambrosio · 2016-12-18T22:10:05Z

examples/FizzBuzz/features/step_definitions/FizzBuzzSteps.cpp

+#include <cgreen/mocks.h> // mocking functionality
+
+#include <cucumber-cpp/autodetect.hpp>
+#include <cucumber-cpp/internal/CukeEngine.hpp>


This should not be included

paoloambrosio · 2016-12-18T22:12:36Z

include/cucumber-cpp/internal/drivers/CgreenDriver.hpp

+#ifndef CUKE_CGREENDRIVER_HPP_
+#define CUKE_CGREENDRIVER_HPP_
+
+#include <cgreen/cgreen.h> // general unit testing


Is the comment needed?

nope, will remove

paoloambrosio · 2016-12-18T22:17:37Z

tests/integration/drivers/CgreenDriverTest.cpp

@@ -0,0 +1,63 @@
+#include <cgreen/cgreen.h> // general unit testing


Same as before

paoloambrosio · 2016-12-18T22:18:51Z

tests/integration/drivers/CgreenDriverTest.cpp

+ DriverTest::runAllTests();
+ }
+private:
+ void stepInvocationInitsBoostTest() {


Not boost :-)

paoloambrosio · 2016-12-18T22:21:32Z

src/drivers/CgreenDriver.cpp

+ __FILE__,
+ __LINE__,
+ "currentTest",
+ arguments);


I'm struggling to follow what this is doing, but I don't think __FILE__ and __LINE__ can work here. Given that it's not in a macro, I believe they would be CgreenDriver.cpp and 145.

Hmm, I'm unclear what the concern here is. __FILE__ and __LINE__ are standard predefined macros that report source file name and line number, respectively. They are used here to give the cgreen reporter the info it requires for the proper output.

I can't understand what this is doing:

va_list arguments; memset(&arguments, 0, sizeof(va_list)); cgreenInterceptor.cgreenReporter->show_fail( cgreenInterceptor.cgreenReporter, __FILE__, __LINE__, "currentTest", arguments);

Even if I remove the entire block, CgreenDriverTest passes and the FizzBuzz example works (even failures are reported correctly if I introduce errors in the code).

shnewto · 2016-12-29T18:18:15Z

@paoloambrosio good catch on that show_fail function, my impression was that I needed to call it to fill the runner output with the correct strings but I tested it as well and it seems not to be the case! I also merged in you're find package solution, thanks!

shnewto · 2017-03-20T20:17:07Z

Recently resolved conflicts in the toml and added some Calc / C++ examples. Any more concerns to address?

konserw · 2017-03-30T10:11:41Z

Does cgreen support building with cmake? couldn't we use externalproject for it?

shnewto · 2017-04-07T21:22:58Z

@konserw Cgreen does build with CMake. Your suggestion is a possibility but I followed the Boost / Qt lead, require a Cgreen install and use find_package to do the legwork. Is that impractical?

paoloambrosio

@Snewt can you rebase to master instead of merging from master? As you do it, I'd squash all commits like "whitespace commit", "removing travis reference to stale branch used during testing", "added a missing newline at the end of .travis.yml file", as well as the merges from Master.

paoloambrosio · 2017-04-08T07:30:34Z

.travis.yml

@@ -22,13 +22,18 @@ matrix:
 compiler: gcc
 env: GMOCK_VER=1.8.0 VALGRIND_TESTS=ON

+before_script:
+ - git clone https://github.com/cgreen-devs/cgreen.git


Given that after several months there is still no sign of a 1.1.0 release, I think we should fix our build to a specific git hash. I want to avoid our build failing when someone makes incompatible changes to Cgreen.

konserw · 2017-04-10T12:53:35Z

@konserw Cgreen does build with CMake. Your suggestion is a possibility but I followed the Boost / Qt lead, require a Cgreen install and use find_package to do the legwork. Is that impractical?

What I had in mind is approach simmilar to what we have for gtest/gmock, especially since there is no cgreen package available for ubuntu (which we use in CI), i.e. if cgreen is available in system use that, otherwise use externalproject to clone and build cgreen for cucumber.
Of course it is only my opinion, what do you think @paoloambrosio ?

paoloambrosio · 2017-04-15T13:11:58Z

I'm not a fan of externalproject so I'm happy to merge this PR with the current CMake file.

paoloambrosio · 2017-04-16T08:07:31Z

CMakeLists.txt

+
+#
+# Valgrind
+#


The build errors are due to this section being duplicated (suspect during rebasing conflict).

paoloambrosio · 2017-04-16T08:23:19Z

For your next PR, I suggest rebasing earlier instead of merging from master. Apologies for not spotting it earlier: it could have saved you a headache. You should be able to solve that problem with the following instructions.

Start an intereactive rebase from master: git rebase -i master and set the second commit for editing:

pick ed59fec cucumber-cpp support for cgreen test framework.
edit 883984f Added Cgreen Calc and CalcQt examples.
pick 60a367f checkout known good cgreen state before build

Unstage changes: git reset HEAD^

Edit CMakeLists.txt removing the second Valgrind section.

Commit the changes with git commit -a and the original message:

    Added Cgreen Calc and CalcQt examples.
    
    In order to show how the Cgreen framework can be used with C++,
    Cgreen tests, modeled after the Boost tests were written
    for the Calc and CalcQt examples.

Continue rebasing: git rebase --continue

shnewto · 2017-04-16T21:49:39Z

@paoloambrosio thanks for the pointers, rebase rather that merge is a workflow I need to master. Also, thanks for putting the time into spotting the duplicate code / bad rebase conflict resolution.

shnewto · 2017-04-18T01:45:36Z

@paoloambrosio was experimenting with a fresh VM tonight and realized that the cgreen package is no longer found on Linux, the OS X builds on Travis run the cgreen examples but the linux builds do not. I think I've spotted the culprit (cgreen_FOUND vs CGREEN_FOUND), will try and fix in the next couple days

shnewto · 2017-04-18T04:35:40Z

Fixed thefind_package(cgreen) issue, examples and tests are running on OS X and Linux builders now but it looks like I have a memory leak to pin down.

shnewto · 2017-04-27T17:39:27Z

update, I have a PR up on the cgreen repository to fix the leak that is causing my the cgreen valgrind tests to fail.

shnewto · 2017-04-27T21:22:39Z

Okay, the Cgreen team has merged my PR. Had to add a few lines to the CgreenDriver source after bumping the version to account for some new error message output but it looks like we actually have a known good state in the hash we checkout in the travis file.

shnewto · 2017-04-27T21:43:44Z

hmm, a couple AppVeyor builds are failing with a FindBoost.cmake error, not sure how to tackle that one. @paoloambrosio I'm definitely open to suggestions.

shnewto · 2017-04-27T23:59:09Z

It looks like the variables in the appveyor.yml passed as arguments to cmake don't like \ in paths. In a test branch on my fork I modified all the appveyor.yml variables to use / in paths instead, see diff here and appveyor success here. The only diff on that branch and this PR is that appveyor.yml file. Seems out of the scope of this PR and very weird that it's only causing an issue now but I can add those changes to this PR if you're interested @paoloambrosio ?

muggenhor

I've tried to look at most of your code changes. I'm guessing you're more experienced at C than C++, so have tried to write my comments in such a way that they don't depend too much on assumed C++ knowledge. Overall it looks like you're on the right track. Please don't be disheartened by the amount of comments I have. Most of them are about details that should be relatively easy to address individually, although I do recognize there's a large number of them.

muggenhor · 2017-04-28T13:00:39Z

.travis.yml

@@ -22,13 +22,19 @@ matrix:
 compiler: gcc
 env: GMOCK_VER=1.8.0 VALGRIND_TESTS=ON

+before_script:
+ - git clone https://github.com/cgreen-devs/cgreen.git
+ - pushd cgreen && git checkout 7030597 && popd


You can also just use git -C cgreen checkout $committish.

muggenhor · 2017-04-28T13:03:28Z

.travis.yml

+before_script:
+ - git clone https://github.com/cgreen-devs/cgreen.git
+ - pushd cgreen && git checkout 7030597 && popd
+ - mkdir cgreen/build && pushd cgreen/build && cmake .. && make && sudo make install && popd


I would prefer for this to be similar to the way we build cucumber-cpp itself, e.g.:

cmake -E make_directory cgreen/build cmake -E chdir cgreen/build cmake -G Ninja .. cmake --build cgreen/build cmake --build cgreen/build --target install

Additionally, the root access (sudo) shouldn't be necessary. If instead you alter the cmake configuration command to be:

cmake -E chdir cgreen/build cmake -G Ninja -DCMAKE_INSTALL_PREFIX=${HOME}/usr ..

Then you can pass -DCMAKE_PREFIX_PATH=${HOME}/usr to the cmake invocation for cucumber-cpp itself and it should just find it.

muggenhor · 2017-04-28T13:07:27Z

CMakeLists.txt

+# cgreen
+#
+
+if(NOT CUKE_DISABLE_CGREEN)


Please add an option near the top of this file like there exists for other optionally disabled features.

muggenhor · 2017-04-28T13:18:23Z

README.md

@@ -27,6 +27,8 @@ It relies on a few libraries:
 Optional library for Boost Test driver: *test*.
 * [GTest](http://code.google.com/p/googletest/) 1.6 or later.
 Optional for the GTest driver. By default downloaded and built by CMake.
+* [Cgreen](https://github.com/cgreen-devs/cgreen) 1.1.0 or later.


Seems like this comment should be marked in some way to indicate that it's instead expected to work with what will eventually become cgreen 1.1.0

muggenhor · 2017-04-28T13:29:44Z

cmake/modules/Findcgreen.cmake

+if(CGREEN_FOUND)
+ set(CGREEN_LIBRARIES ${CGREEN_LIBRARY})
+ set(CGREEN_INCLUDE_DIRS "${CGREEN_INCLUDE_DIR}")
+ set(CGREEN_EXECUTABLE "${CGREEN_RUNNER}")


I would prefer it if instead you would add import targets here, similar to what we do in FindGMock.cmake. That should make it significantly easier to actually use this.

For example this (untested, but it should be in the right direction):

add_library(Cgreen::Cgreen UNKNOWN IMPORTED) set_target_properties(Cgreen::Cgreen PROPERTIES IMPORTED_LINK_INTERFACE_LANGUAGES "C" IMPORTED_LOCATION "${CGREEN_LIBRARY}" INTERFACE_INCLUDE_DIRECTORIES "${CGREEN_INCLUDE_DIR}" ) add_executable(Cgreen::runner IMPORTED) # This may need the GLOBAL keyword as well, needs to be tested to confirm either way set_target_properties(Cgreen::runner PROPERTIES IMPORTED_LOCATION "${CGREEN_RUNNER}" )

Then just use those two targets where currently you use these intermediate variables instead. (Uses of the ${CGREEN_INCLUDE_DIRS} variable can be removed, as they're now covered by the usage requirements in the Cgreen::Cgreen target properties).

muggenhor · 2017-04-28T14:15:01Z

src/drivers/CgreenDriver.cpp

+
+bool CgreenStep::initialized = false;
+
+boost::function< void() > currentTestBody;


Why don't you make this a static class member instead?

muggenhor · 2017-04-28T14:18:53Z

tests/integration/drivers/CgreenDriverTest.cpp

+ }
+private:
+ void stepInvocationInitsCgreenTest() {
+ std::cout << "= Init =" << std::endl;


Unless you really want .flush() to be called here I suggest you just terminate the string with "\n" instead of streaming in std::endl. std::endl does nothing more than streaming in "\n" followed by calling .flush().

FYI: some people think it takes care of converting to the current OS' end-of-line character sequence: it doesn't, that's what the underlying stream itself already does for "\n" (exactly like not passing the "b" flag to fopen() does for C, in fact: on GNU's libstdc++ it uses the C library).

muggenhor · 2017-04-28T14:21:18Z

travis.sh

@@ -31,3 +32,7 @@ if [ -f $BOOST ]; then
 cucumber examples/Calc
 wait
 fi
+if [ -f $CGREEN ]; then
+ $CGREEN >/dev/null &
+ cucumber examples/FizzBuzz


Please add a wait call after cucumber. That makes sure that to wait for $CGREEN to terminate (and to use its exit code).

muggenhor · 2017-04-28T14:22:58Z

src/drivers/CgreenDriver.cpp

+ va_list argPtr;
+
+ va_start(argPtr, format);
+


I see this a lot: empty lines between related code. Could you try to remove some of that?

In this case, for example, I wouldn't expect to see a single empty line between the va_list declaration and the va_end call, as it's all related and logically doing one thing.

muggenhor · 2017-04-28T14:26:37Z

src/drivers/CgreenDriver.cpp

+
+static CukeCgreenInterceptor cgreenInterceptor;
+
+std::string CukeCgreenInterceptor::cgreenOutput = std::string();


Please remove the copy-initialization. In C++ all objects of types that have a default constructor (which is almost all non-POD types) will get it called when no initializer is specified (so no need to worry about uninitialized memory here).

shnewto · 2017-04-28T20:19:16Z

@muggenhor thanks for all the great feedback, the changes you requested are well justified. I really appreciate you taking the time to explain your reasoning.

shnewto · 2017-04-28T21:05:36Z

@muggenhor I've tried to address your requested changes. Thanks again for the comments, I learned some things implementing them. I'm unsure about my README.md wording still, I don't know the best way to indicate a supported cgreen version when there hasn't been an actual version in some time. I've been using cgreen and cucumber-cpp in tandem for some time now and feel like even at the stage of development cgreen is currently at it is still a valuable tool. It didn't seem like a commit hash in the README was appropriate though.

shnewto · 2017-04-28T21:06:34Z

Also, it appears the / v \ in variables in the appveyor builds is still causing problems.

shnewto · 2017-04-28T22:44:55Z

I kicked off another appveyor build of branch that differs from this one only in the appveyor.yml where \ in variable paths have been changed to /. You can check out the file diff here. and the build status here.. Let me know what you'd like me to do regarding those failures on this PR.

muggenhor · 2017-06-02T09:51:17Z

cmake/modules/Findcgreen.cmake

+ set_target_properties(Cgreen::runner PROPERTIES
+ IMPORTED_LOCATION "${CGREEN_RUNNER}"
+ )
+ include_directories(${CGREEN_INCLUDE_DIR})


Please delete this line: we don't want to unconditionally add this to the include path of everything.

muggenhor · 2017-06-02T09:57:06Z

examples/FizzBuzz/CMakeLists.txt

+ add_executable(FizzBuzzSteps features/step_definitions/FizzBuzzSteps)
+ target_link_libraries(FizzBuzzSteps FizzBuzz ${CUKE_LIBRARIES} Cgreen::Cgreen)
+else()
+ message("cgreen package required for FizzBuzz example")


Please just don't: we don't do this for any of the other test-framework-specific tests and executables either, so lets not start doing this here. It just increases the log verbosity with messages most people will tend to ignore.

muggenhor · 2017-06-02T09:59:10Z

examples/FizzBuzz/src/FizzBuzzReporter.h

@@ -0,0 +1,11 @@
+#ifdef __cplusplus


Add an include guard to this header, e.g.:

#ifndef CUKE_FIZZBUZZREPORTER_H_ #define CUKE_FIZZBUZZREPORTER_H_ // contents #endif

muggenhor · 2017-06-02T09:59:26Z

examples/FizzBuzz/src/FizzBuzzReporter.h

+extern "C" {
+#endif
+
+#include <string.h>


You're not using anything from this header: don't include it here.

muggenhor · 2017-06-02T10:00:54Z

examples/FizzBuzz/src/FizzBuzzReporter.h

+
+#include <string.h>
+
+void fizzBuzzReporter(unsigned int input, char * reportBuffer, size_t bufferSize);


size_t depends on #include <stddef.h>, please do

muggenhor · 2017-06-02T10:07:34Z

src/drivers/CgreenDriver.cpp

+std::string CukeCgreenInterceptor::cgreenOutput;
+TestSuite * CukeCgreenInterceptor::cgreenSuite = NULL;
+TestReporter * CukeCgreenInterceptor::cgreenReporter = NULL;
+boost::function< void() > CukeCgreenInterceptor::currentTestBody = NULL;


this is not a pointer, just leave it default initialized (i.e. remove the = NULL bit).

muggenhor · 2017-06-02T10:12:36Z

src/drivers/CgreenDriver.cpp

+ cgreenOutput += buffer;
+ cgreenOutput += "\n\n";
+ }
+ va_end(argPtr);


Please move the va_end to directly after vsnprintf.

shnewto · 2017-06-02T23:31:05Z

@muggenhor regarding the request to remove the blanketed include_directories(${CGREEN_INCLUDE_DIR}). I removed that from the Findcgreen.cmake file but then reverted the CMakeLists.txt files that build cgreen dependent targets to use target_include_directories(target_name PRIVATE ${CGREEN_INCLUDE_DIR}). We don't have to do the same for Boost or Gtest because their headers are in a system visible location right? If you or @paoloambrosio have an approach you'd prefer to the one I've taken I'm certainly open to suggestions.

muggenhor

You don't need to add the target_include_directories calls. You just seem to have forgotten to ensure that the cucumber-cpp target itself gets linked to Cgreen::Cgreen. One of my review comments (in src/CMakeLists.txt) explains how to fix that.

muggenhor · 2017-06-03T11:55:47Z

.travis.yml

+ - cmake -E chdir cgreen/build cmake -G Ninja -DCMAKE_INSTALL_PREFIX:PATH=${HOME}/usr ..
+ - cmake --build cgreen/build
+ - cmake --build cgreen/build --target install
+ - if [[ "$TRAVIS_OS_NAME" == "linux" ]]; then sudo ldconfig; fi


Oh btw, this (ldconfig) can go: we're not installing in a system-wide directory anymore.

muggenhor · 2017-06-03T12:06:21Z

src/CMakeLists.txt

@@ -24,6 +24,10 @@ if(Boost_UNIT_TEST_FRAMEWORK_FOUND)
 list(APPEND CUKE_SOURCES drivers/BoostDriver.cpp)
 endif()

+if(CGREEN_FOUND)
+ list(APPEND CUKE_SOURCES drivers/CgreenDriver.cpp)


Just like is done for GTest (few lines above): you need to ensure Cgreen gets linked to, like this:

list(APPEND CUKE_DEP_LIBRARIES Cgreen::Cgreen)

shnewto · 2017-06-03T21:17:44Z

@muggenhor ah thanks, I'd misremembered.

muggenhor · 2017-06-03T21:42:40Z

src/drivers/CgreenDriver.cpp

+
+class CukeCgreenInterceptor {
+public:
+ const std::string getCgreenOutput();


Two options:

Remove the const that will allow C++11 (and beyond) compilers to perform a (cheaper) move instead of copy

Return a const reference instead, thus postponing the copy until it's really necessary

muggenhor · 2017-06-03T21:49:56Z

src/drivers/CgreenDriver.cpp

+ cgreenInterceptor.reset_reporter();
+ }
+
+ cgreenInterceptor.currentTestBody = boost::bind(&CgreenStep::body, this);


You can change the type of currentTestBody to BasicStep*. Then you can just call currentTestBody->body() inside of currentTest. That's a lot more light-weight than using boost::bind and boost::function.

The body() function of the BasicStep class is protected.

muggenhor · 2017-06-03T21:53:59Z

tests/integration/drivers/CgreenDriverTest.cpp

+ }
+private:
+ void stepInvocationInitsCgreenTest() {
+ std::cout << "= Init =\n";


Please remove this line

muggenhor · 2017-06-03T21:56:31Z

src/drivers/CgreenDriver.cpp

+
+ va_list argPtr;
+ va_start(argPtr, format);
+ vsnprintf(buffer, sizeof(buffer), format, argPtr);


capture the return value and verify that it's not negative before using buffer

muggenhor · 2017-06-03T21:59:07Z

src/drivers/CgreenDriver.cpp

+}
+
+CukeCgreenInterceptor::~CukeCgreenInterceptor() {
+ destroy_test_suite(cgreenSuite);


As I see it: this will always be called by cucumber (because you've made it a global variable), but you're not always initializing these to be non-NULL. So please ensure that you're not destroying NULL test suites and reporters.

muggenhor · 2017-06-03T22:02:39Z

src/drivers/CgreenDriver.cpp

+ "/"
+};
+
+class CukeCgreenInterceptor {


This class smells: it's basically a singleton (the single static instance of this class) with every member being a singleton too (static member variables)! I suspect the quality of your destructor (which to me looks like it will trigger NULL pointer dereferences) is the result of this.

Please:

do without any singleton at all, or;

don't nest singletons and make the top-level one closer to the singleton design pattern with lazy initialization (use a smart pointer for the instance pointer to ensure the destructor actually gets called).

Revised to conform to my understanding of what a lazy initialized singleton looks like. The project doesn't seem to enforce c++11 so I can't assume std::unique_ptr is available.

Correct, you should assume only C++98, as we currently still are able to build with just that. You don't need std::unique_ptr for a singleton though, that just makes it slightly easier to implement.

@muggenhor got it. Well I took a stab at implementing the lazy evaluate singleton you requested without the unique_ptr. I'm not dynamically allocating memory for the singleton itself though there are fields in it that are allocated and freed in its destructor. My assumption is that that memory is being successfully managed since the Valgrind tests pass. Let me know what your thoughts are.

shnewto · 2017-09-20T00:21:32Z

I've been unable to reproduce the BoostCalculatorQtSteps failure that's breaking the Travis/Linux builds locally but am investigating. I triggered a build of the current state of master from my fork and it seems to be broken there too.

konserw · 2017-09-20T10:02:29Z

It's something with travis image itself - see #170 #169 I'm not sure what can we do about that

konserw · 2017-10-13T09:00:08Z

@Snewt - FYI if you rebase now onto master travis build should pass (issue has been worked-around in master)

paoloambrosio · 2017-10-13T16:53:41Z

Don't really like merges from master. Rebase is your friend, like @konserw suggested.

As a user I want to use cgreen as an alternative test framework to gtest and boost in order to test native C complete with mocking capability. cucumber-cpp now recognizes and harnesses cgreen as a testing framework. The cgreen reporting is intercepted and presented on the failure of a cucumber test. An example (FizzBuzz) is written as an example of a testable C progam and is included in the examples directory. Unit tests for the cgreen driver are modeled after the boost and gtest drivers and are passing.

In order to show how the Cgreen framework can be used with C++, Cgreen tests, modeled after the Boost tests were written for the Calc and CalcQt examples.

Prior to this commit the version of Cgreen checked out in the `.travis.yml` file introduced a memory leak to the CgreenDriver. After this commit the version of Cgreen checked out by Travis reflects a version of Cgreen with that memory leak fixed. Due to the bump in Cgreen version that introduced new cgreen error output, some "black box" functionality of the cucumber-cpp CgreeDriver was being reported. This commit also truncates that information so it isn't visible in a failed tests' error output.

konserw · 2017-10-13T19:30:50Z

travis.sh

 ; do
 if [ -f "${TEST}" -a -n "${DISPLAY:-}" ]; then
- "${TEST}" 2> /dev/null &


Don't change this line - it will produce a lot of garbage if you change it

konserw · 2017-10-13T19:31:23Z

travis.sh

@@ -92,4 +107,7 @@ if [ -f "${TEST}" ]; then
 wait %
 fi

-killXvfb


also don't change this one (and following)

shnewto · 2017-10-16T17:27:07Z

@konserw thanks for pointing those issues out. Looks like my build is still failing though. Any more suggestions? I experimented with another branch and the failure seems to be rooted in this block of the travis.sh

SOCK=cucumber.wire.sock
  TEST=build/examples/FeatureShowcase/FeatureShowcaseSteps
 if [ -f "${TEST}" ]; then
     echo "unix: ${SOCK}" > examples/FeatureShowcase/features/step_definitions/cucumber.wire
     "${TEST}" --unix "${SOCK}" > /dev/null &
     cucumber examples/FeatureShowcase
     wait %
 fi

Prior to this commit this feature's implementation did not fully conform to C++ standards and convention. This lead to inconsistencies and ambiguity in the source. After this commit the implementation should conform somewhat more to this project's standards as well as a C++ convention. Other changes included in this commit: - cmake revisions to match the functionality and usage for this project's gmock dependency. - travis file cleanup, the logic applied to Cgreen steps did not fully conform to the project's other dependencies. - Documentation regarding supported Cgreen versions (or more accurately lack of versions)

…licts

…ke issues

jermus67 · 2021-09-17T21:37:13Z

Hallo,

Despite really appreciating the effort the @shnewto has put into this PR, I also think it is not a good direction to add support for various (unit) test frameworks to cucumber-cpp. This would make cucumber-cpp dependent on each and every test framework it is embedded in, while it should be the other way around: the test of the user of cucumber-cpp should be depending on both test framework it runs on and cucumber-cpp, the dependency for cucumber-cpp towards the test framework should be injected by the user of the test by means of a - by cucumber-cpp - well defined interface (kind of as describe in issue #154 ).

This PR will be closed without being merged into main (besides, this PR seems to be really out of date).

However, contributors to this PR are more than welcome to participate in the design of a plugin / dependency injection system for test drivers in cucumber-cpp.

Kind regards,
Jeroen Kouwer

paoloambrosio reviewed Dec 18, 2016

View reviewed changes

paoloambrosio requested changes Apr 8, 2017

View reviewed changes

paoloambrosio reviewed Apr 16, 2017

View reviewed changes

muggenhor requested changes Apr 28, 2017

View reviewed changes

muggenhor requested changes Jun 2, 2017

View reviewed changes

muggenhor requested changes Jun 3, 2017

View reviewed changes

muggenhor mentioned this pull request Jun 3, 2017

Design a plugin(-like) architecture for test drivers #154

Open

muggenhor requested changes Jun 3, 2017

View reviewed changes

snewton and others added 4 commits October 13, 2017 10:44

Added Cgreen Calc and CalcQt examples.

f836357

In order to show how the Cgreen framework can be used with C++, Cgreen tests, modeled after the Boost tests were written for the Calc and CalcQt examples.

checkout known good cgreen state before build

68ecb47

konserw reviewed Oct 13, 2017

View reviewed changes

shea newton added 3 commits January 22, 2018 16:19

update feature / PR branch to cucumber-cpp master, fix resulting conf…

94e5a09

…licts

update README with Cgreen reference, resolve travis.sh and CalcQt CMa…

51705a6

…ke issues

mattwynne mentioned this pull request May 6, 2021

Consistent stalebot rules cucumber/common#1523

Open

jermus67 closed this Sep 17, 2021

		@@ -0,0 +1,63 @@
		#include <cgreen/cgreen.h> // general unit testing


		bool CgreenStep::initialized = false;

		boost::function< void() > currentTestBody;


		static CukeCgreenInterceptor cgreenInterceptor;

		std::string CukeCgreenInterceptor::cgreenOutput = std::string();


		#include <string.h>

		void fizzBuzzReporter(unsigned int input, char * reportBuffer, size_t bufferSize);

cucumber-cpp support for cgreen test framework. #129

cucumber-cpp support for cgreen test framework. #129

Conversation

shnewto commented Nov 16, 2016

shnewto commented Nov 18, 2016

paoloambrosio commented Nov 22, 2016 • edited Loading

shnewto commented Nov 23, 2016

paoloambrosio commented Nov 25, 2016 • edited Loading

aslakhellesoy commented Nov 25, 2016

shnewto commented Nov 25, 2016

shnewto commented Nov 25, 2016

aslakhellesoy commented Nov 25, 2016

paoloambrosio commented Dec 8, 2016 • edited Loading

paoloambrosio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shnewto commented Dec 29, 2016

shnewto commented Mar 20, 2017

konserw commented Mar 30, 2017

shnewto commented Apr 7, 2017

paoloambrosio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konserw commented Apr 10, 2017

paoloambrosio commented Apr 15, 2017

Choose a reason for hiding this comment

paoloambrosio commented Apr 16, 2017

shnewto commented Apr 16, 2017

shnewto commented Apr 18, 2017 • edited Loading

shnewto commented Apr 18, 2017

shnewto commented Apr 27, 2017

shnewto commented Apr 27, 2017

shnewto commented Apr 27, 2017

shnewto commented Apr 27, 2017

muggenhor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shnewto commented Apr 28, 2017

shnewto commented Apr 28, 2017

shnewto commented Apr 28, 2017

shnewto commented Apr 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shnewto commented Jun 2, 2017

muggenhor left a comment • edited Loading

Choose a reason for hiding this comment

muggenhor Jun 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shnewto commented Jun 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paoloambrosio commented Nov 22, 2016 •

edited

Loading

paoloambrosio commented Nov 25, 2016 •

edited

Loading

paoloambrosio commented Dec 8, 2016 •

edited

Loading

shnewto commented Apr 18, 2017 •

edited

Loading

shnewto commented Apr 28, 2017 •

edited

Loading

muggenhor left a comment •

edited

Loading

muggenhor Jun 3, 2017 •

edited

Loading

shnewto Aug 14, 2017 •

edited

Loading

shnewto commented Sep 20, 2017 •

edited

Loading

konserw Oct 13, 2017 •

edited

Loading